Skip to content

Fix segfault when failing to instantiate Wasm module.#240

Merged
PiotrSikora merged 16 commits intoproxy-wasm:masterfrom
PiotrSikora:fail-return
Feb 8, 2022
Merged

Fix segfault when failing to instantiate Wasm module.#240
PiotrSikora merged 16 commits intoproxy-wasm:masterfrom
PiotrSikora:fail-return

Conversation

@PiotrSikora
Copy link
Member

@PiotrSikora PiotrSikora commented Feb 2, 2022

Signed-off-by: Piotr Sikora piotrsikora@google.com

@PiotrSikora
Fix segfault when failing to instantiate Wasm module.
cfc5690 
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora PiotrSikora marked this pull request as ready for review February 2, 2022 12:56
mathetake
mathetake reviewed Feb 2, 2022
View reviewed changes
Copy link
Contributor

@mathetake mathetake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you want to add tests (as I see we have tests for all runtime)?

@PiotrSikora
review: increase the maximum number of WasmVMs when using V8.
50a0074 
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora
review: fix segfault when faling to clone WAVM.
7318259 
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora
review: add test.
74e6d4d 
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora
review: fix format.
d40104b 
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora
review: add speculative fix for WAVM.
8a76ad7 
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora
review: fix typo.
f060e3d 
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora PiotrSikora mentioned this pull request Feb 7, 2022
Closed
@PiotrSikora
review: skip WAVM on the CI.
8f34ce0 
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora
Revert "review: skip WAVM on the CI."
0bbbc29 
This reverts commit 8f34ce0.

Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora
review: check failures in load(), link() and clone().
861a798 
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora
review: ASSERT_GE() -> EXPECT_GE().
fa9ee1f 
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora
Revert "Revert "review: skip WAVM on the CI.""
7269ac2 
This reverts commit 0bbbc29.

Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora
Copy link
Member Author

PiotrSikora commented Feb 7, 2022

@mathetake in my attempt to fix WAVM on the CI, I've added checks for all (?) failures in load(), link() and clone(), other than those in stlib allocations. We originally skipped them, since they go against Envoy's coding style, but I think it makes sense to have them now. PTAL.

Unfortunately, this didn't fix the test with WAVM on the CI. I suspect that it fails because of a failure in emplace running out of memory or something like that, but since it passes locally, and it's not the primary runtime, I'm going to skip it for now.

@PiotrSikora
Copy link
Member Author

PiotrSikora commented Feb 7, 2022

@mathetake in my attempt to fix WAVM on the CI, I've added checks for all (?) failures in load(), link() and clone(), other than those in stlib allocations. We originally skipped them, since they go against Envoy's coding style, but I think it makes sense to have them now. PTAL.

Done. Good call, thanks!

mathetake
mathetake approved these changes Feb 7, 2022
View reviewed changes
Copy link
Contributor

@mathetake mathetake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@PiotrSikora
review: increase test timeout for macOS.
6f4dbae 
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora
Revert "review: increase test timeout for macOS."
8b184a6 
This reverts commit 6f4dbae.

Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora
review: execute this test only on Linux/x86_64.
87d94e3 
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
@PiotrSikora
Merge branch 'master' into fail-return
d7c4e02
@PiotrSikora PiotrSikora merged commit 9699b32 into proxy-wasm:master Feb 8, 2022
@PiotrSikora PiotrSikora mentioned this pull request Feb 11, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@mathetake mathetake mathetake approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@PiotrSikora @mathetake

Comments